Add the name resolver API #2285
Conversation
arjan-bal
force-pushed
the
nameresolver-api
branch
from
2867b3f to
3c30ab8
Compare
arjan-bal
force-pushed
the
nameresolver-api
branch
from
3c30ab8 to
07eb017
Compare
|
|
||
| /// The delay for the next retry, without the random jitter. Store as f64 | ||
| /// to avoid rounding errors. | ||
| next_delay_secs: Mutex<f64>, |
There was a problem hiding this comment.
Why does this need a mutex? It seems like we should generally only be accessing these serially.
There was a problem hiding this comment.
You're correct access is serial. The mutex is added to allow interior mutability. @easwars and I have a discussion about this on the group. The subchannel will also use backoffs and it will pass the backoff as an immutable value to keep the subchannel API clean. To get rid of the mutex we would need to use mutable acceptors in the trait definition and have the subchannel wrap the backoff in a mutex to get a mutable ref.
There was a problem hiding this comment.
It would be better to extend the mut receiver out as far as we can. The mutex can go into the subchannel if that's what it needs to do. The backoff can express that it should not be called concurrently by making this a mutable receiver instead of an immutable one. The behavior of concurrent accesses would be undefined anyway.
| /// returns an empty endpoint list but a valid service config may set | ||
| /// to this to something like "no DNS entries found for <name>". | ||
| pub resolution_note: Option<String>, |
There was a problem hiding this comment.
Is there some requirement for using this? Like should it only be set if endpoints is empty? I.e. should we delete this and use an Err() from endpoints instead?
There was a problem hiding this comment.
From my understanding, the resolution_note can be set when endpoints are present. The resolution note may say something like "the endpoints are stale" and the LB policy will add this message to the failure message if it fails to connect to the endpoints. Here is the code in CPP that sets a resolution node with a non-empty list of addresses.
| pub resolution_note: Option<String>, | ||
| } | ||
|
|
||
| impl Default for ResolverUpdate { |
There was a problem hiding this comment.
Can this be derived? Or no because of Result? If the latter, is there some way to partially derive?
There was a problem hiding this comment.
It can't be derived directly due to the Result. We can't implement the Default trait for Result<T, E> because Result is from a foreign crate. We can wrap Result<Option<ServiceConfig>, String> in our own type, but that would add an extra layer for accessing the ServiceConfig/Entpoints etc.
I couldn't find a way to avoid listing the fields that already implement Default without adding a crate with macros. The closest think I found is using Default::default() as the value for such fields (e.g: attributes: Arc::default()). Using ..Default::default() at the end of the struct in the implementation of default() causes infinite recursion.
No description provided.